Initial Rust implementation #174
Conversation
|
The files are intended to be compared side-by-side with the C++ implementation. E.g., script.rs first contains definitions from script.h, followed by definitions from script.cpp. |
sellout
force-pushed
the
sellout/rust-interpreter
branch
from
629eed7 to
e95972b
Compare
The only changes here other than moving chunks of code around are - moving `evaluate` out of `impl Script`, which required changing `&self` to `script: &Script`; and - unifying `ExecutionOptions` with `VerificationFlags`.
For easier side-by-side comparison.
sellout
force-pushed
the
sellout/rust-interpreter
branch
3 times, most recently
from
848ebaa to
09ac96b
Compare
The C++ impl uses exceptions for `ScriptNum`, but catches them.
These tests run both the C++ and Rust impls. One uses completely arbitrary inputs, and the other limits script_sig to data pushes.
For now, the underlying errors are discarded when comparing against the C++ results, but there are corresponding changes on the C++ side in a separate branch.
sellout
force-pushed
the
sellout/rust-interpreter
branch
from
09ac96b to
4c34b3e
Compare
|
I don’t know why the test suite is failing on the C++ side now.
My only guess is that it is somehow caused by the secp256k1(_sys) dependency maybe forcing a different version of the underlying Since there’s a lot of noise in the logs, here’s the actual error: |
It works on Rust 1.81 but it doesn't on 1.82. Which is super weird, but should give you a direction where to investigate |
- remove `uint256` module - create a specific type for the C++/Rust comparison implementation - rename some identifiers - rephrase some comments
| Some((hash_type, vch_sig)) => HashType::from_bits((*hash_type).into(), false) | ||
| .ok() | ||
| .and_then(|hash_type| (self.sighash)(script_code.0, hash_type)) | ||
| .map(|sighash| Self::verify_signature(vch_sig, &pubkey, &sighash)) | ||
| .unwrap_or(false), |
There was a problem hiding this comment.
This doesn't seem right. In the C++, TransactionSignatureChecker::CheckSig calls SignatureHash. In the SIGVERSION_ZIP244 case when txTo.IsCoinBase() || txTo.vin.empty() is false, it throws a std::logic_error in the case of an undefined hash_type (not 0x01, 0x02, 0x03, 0x81, 0x82, or 0x83).
HashType::from_bits((*hash_type).into(), false), on the other hand, will only return an error if the two least significant bits are invalid. So there is a divergence in the ZIP 244 case when an undefined bit is set in the hash type.
There was a problem hiding this comment.
As discussed the other day, this is a divergence between zcashd’s zcash_script and Zebra’s C++ zcash_script (in this repo), largely due to #157.
The Zebra version doesn’t call SignatureHash.
There was a problem hiding this comment.
So, I don’t think there’s anything to do here – this matches the callback-based C++ as used by Zebra.
There was a problem hiding this comment.
I believe that needs to be enforced by the caller, in the callback, since this crate is not aware of whether it's a NU5-onward tx. I'll make a note to double check this in Zebra.
There was a problem hiding this comment.
It’s on the caller to include the StrictEnc flag for NU5 tx1, it doesn’t need to be handled in the callback (and can’t be because the callback receives a HashType, not the bits)2.
If that flag is set, then from_bits gets called with is_strict: true from
zcash_script/src/interpreter.rs
Lines 386 to 392 in 2afc474
zcash_script/src/interpreter.rs
Lines 61 to 63 in 2afc474
It’s annoying to follow in this match-the-C++ PR, because each call to from_bits has is_strict hardcoded, but in a future commit is_strict is set directly from the StrictEnc flag.
Footnotes
-
I forget if it even made it into master, but there was a brief period when I reintroduced
TxVersion(and a dependency on zcash_primitives) here, so that NU5 transactions would be strict regardless of whetherStrictEncwas set, but that was ultimately shot down. ↩ -
The wrapper used to pass a Rust callback to the C++ implementation explains why it’s ok to be non-strict there https://github.com/ZcashFoundation/zcash_script/blob/2afc4743383686411941fba431f1bf3a8271c059/src/lib.rs#L131-L134 ↩
| .map_err(|_| ScriptError::PubKeyCount)?; | ||
| if keys_count > 20 { | ||
| return set_error(ScriptError::PubKeyCount); | ||
| }; |
There was a problem hiding this comment.
I don’t know about this one, because
- the C++ code doesn’t assert here,
- we do check that and return
ScriptError::OpCountjust below, and - I’m not positive there’s not a bug (in the original) that would result getting here with an invalid
op_countthat leads to divergent results in the stepper (assertion failure vsScriptError).
I haven’t done this yet, but it seems like a future change to not expose op_count directly (a future commit does encapsulate it in a State struct), but have an incrementer that returns Err(ScriptError::OpCount) immediately whenever the bound is violated.
| Some((hash_type, vch_sig)) => HashType::from_bits((*hash_type).into(), false) | ||
| .ok() | ||
| .and_then(|hash_type| (self.sighash)(script_code.0, hash_type)) | ||
| .map(|sighash| Self::verify_signature(vch_sig, &pubkey, &sighash)) | ||
| .unwrap_or(false), |
There was a problem hiding this comment.
As discussed the other day, this is a divergence between zcashd’s zcash_script and Zebra’s C++ zcash_script (in this repo), largely due to #157.
The Zebra version doesn’t call SignatureHash.
* Add ECC & myself (Greg Pfeil) as authors * Have the Rust impl correctly report “high s” sigs There is a bug in the C++ side where the error is not set correctly on a “high s” signature. The Rust side had mirrored this bug, but this eliminates the bug in the Rust. * Remove extra byte from sig before low-s check This doesn’t seem to have any effect on the semantics, as the DER-formatted signature includes lengths that ensure it will ignore extra bytes, but the C++ code removes the extra byte, so the Rust should as well. * Change some comments Co-authored-by: Daira-Emma Hopwood <[email protected]> * Appease `rustfmt` * Have OP_DUP match the C++ impl more closely * Address the second half of @daira’s #174 review * Eliminate mutation from `Opcode` parsing This now splits slices and returns the remaining pieces rather than modifying the arguments. * Remove obsolete comment * Address PR comments * Address additional comments on #174 --------- Co-authored-by: Daira-Emma Hopwood <[email protected]>
This provides two more implementations of the
ZcashScripttrait. One is a pure Rust implementation and the other runs both the C++ and Rust implementations, returning the C++ result and logging a warning if they differ.This follows the C++ implementation extremely closely for ease of initial review. Future changes will move from this to a Rustier approach.
E.g.,
ifinstead ofmatchIt differs from the C++ in some places, as well. Numeric types and casting/From are changed in some places to avoid partiality. Also, the Opcodes are partitioned into multiple types because we eventually want to be able to enforce
push_onlyat the type level, andPushValueoperations carry data (although not yet in the structure), which makes them distinct from operations (PushdataBytelength, in particular).Footnotes
accesses generally return in
Result, so?ensures they’re safe, but we keep the bounds checking so that the stack state matches the C++ version (e.g., without the bounds checking, an op that pops two elements off the stack could fail after popping one, which would leave the stack in a different state than the C++ impl which checks the length of the stack before popping either). ↩